fix(discovery): preserve S3 placement and inference dependencies - #262

🤖 AI Code Review (two independent reviewers)

Reviewer responses: codex/ALL claude/ALL

Status: PASSED — No blocking issues found

PR #262 — fix(discovery): preserve S3 placement and inference dependencies — Chair Synthesis

1. Summary

This PR makes the S3 sync collectors paginate ListBuckets fully and prefer its BucketRegion metadata so a denied GetBucketLocation no longer discards known bucket placement, and adds AgentCore runtime / requested-model nodes to the trace graph as explicitly unverified telemetry claims projected consistently across HTTP, MCP, and a new SQL-reader view migration. Both panels found no CRITICAL or MAJOR issues; the read-only posture, ADR-005 freeze, claim-provenance boundary, and docs/CHANGELOG discipline all hold, with a handful of verified MINOR robustness and disclosure gaps.

Chair verification notes: re is imported in both touched Python modules; the new migration's column list/order exactly matches the queue-provenance view it replaces (so CREATE OR REPLACE VIEW cannot fail on column drift); and the inventory-bridge link remains gated on kind === 'db' (web/app/topology/services/page.tsx:192), so runtime/model nodes cannot navigate into AWS inventory even with a retained infra_ref.

2. Issues per lens

L2 — Code correctness (all MINOR)

• MINOR (claude only, verified in diff) — _list_s3_buckets is stricter than the replaced s3.list_buckets().get("Buckets", []) or []: a response with an absent/null Buckets member now fails the whole s3/s3_public_access sweep (status='failed', prune skipped) instead of publishing an empty-but-complete listing. Only {"Buckets": []} is test-covered; distinguish "key absent ⇒ empty page" from "wrong type ⇒ fail closed".
• MINOR (claude only, plausible) — the repeated-token guard in _list_s3_buckets raises if a page echoes the request's ContinuationToken. The S3 API wording doesn't clearly guarantee a fresh token on the terminal page, so accounts over 1,000 buckets could fail closed every sweep if the token is echoed. Using the botocore paginator or treating an echoed token as end-of-list would remove the assumption. Fail-closed is the safe direction, so this is availability, not integrity.
• MINOR (both panels converged, verified in diff) — in modelClaim() (web/lib/inference-evidence.ts), a present but non-DNS-valid server.address nulls the entire model claim. Codex showed valid IPv6 literals (fd00::10) hit this; claude showed malformed hostnames do too, with no disclosure counter (unlike the messaging path's unresolvedMessaging → partial). Accept IP literals or drop only the endpoint (keeping the caller-local model observation) and/or count the drop.
• MINOR (codex only, confirmed — standard Python semantics) — _inference_claim_meta in agent/lambda/inventory_read_mcp.py uses \d{12} for the account segment; Python \d matches Unicode digits, unlike the TS/SQL [0-9]{12} validators, so a crafted retained ARN could regain a claimed account only through the MCP reader. Use [0-9]{12} for cross-reader parity.
• MINOR (claude only) — the new E2eGraphCanvas detail keys (port, arn, …) combined with the pre-existing nested-endpoint fallback can surface nested network-endpoint fields as new rows on non-inference nodes. The [object Object] case is correctly guarded and tested; the extra rows are benign but unasserted — either intend or scope them.

L3 — Security / AWS mutation safety

No CRITICAL/MAJOR — both panels agree. No Terraform outside an additive migration file; no mutating AWS API (only ListBuckets/GetBucketLocation, both read-only; BucketRegion rides the existing s3:ListAllMyBuckets grant, so "no new IAM" is accurate). The migration grants SELECT only on the sql_reader view, keeps security_invoker = false, and the new test asserts public.topology_nodes stays unreadable. identityProvenance='telemetry_claim' is forced on all three read paths; stored ownership, infra_ref, and prompt/completion content are excluded (asserted by tests). No hardcoded secrets; fixture IDs are synthetic. ADR-005/007 boundaries untouched.

• MINOR (claude only, operational) — CREATE OR REPLACE VIEW + GRANT are outside the automatic-migration subset, so AUTOMATIC_MIGRATION=1 deploys hard-fail until the standalone migration is applied. The new runbooks state this correctly; it's a deliberate contract, flagged only because the CHANGELOG omits it (see L5).

L4 — Observability / data-integration correctness (all MINOR)

• MINOR (claude only, verified in diff) — the hosts edge accrues weight w for every span of the reporting service, so at the 200-node cap runtime-context nodes can outrank genuine low-volume queries/calls dependencies — the very dropout class this release targets. Disclosed via nodeDrops/partial, so it's ranking quality, not silent corruption; weight hosts at 1 or exclude it from the cap sort.
• MINOR (overlaps L2) — dropped malformed inference claims have no counter, so GraphCollectionStatus reads ok for windows where inference dependencies were discarded.
• MINOR (claude only, cosmetic) — a stored JSON port of 1.0 projects as 1.0 through the SQL view but 1 through the TS/Python readers; the shared fixtures (all integer ports) don't catch the drift.

The aggregate SQL changes are sound: inference attribute keys are allowlisted on both resource and span sides, ordinary HTTP peers still collapse, and EC2_CLAIM keeps ARN-valued cloud.resource_id intact so hosts works on the aggregate path (claude verified; no disagreement).

L5 — Docs/ADR consistency

• MINOR (claude flagged; codex found no issue — mild disagreement, claude's is substantiated) — the CHANGELOG bullet describes the new runtime/model observations but never mentions the new 01M7F17PJXXY3P2T3TGDJSF8H7_trace_inference_claim_provenance.sql migration or its required standalone (non-automatic) application, even though docs/api-reference.md itself says identity fields are absent until it's applied and the repo has precedent for naming migrations in release notes. One clause in the existing bullet (amended in place, per the CHANGELOG rule) closes it.

Otherwise both panels agree docs track the implementation: runbook ownership retargeting is accurate (the queue test really does re-apply only the historical migration), api-reference matches code on CLIENT-span requirement/exact-ARN joins/null model qualifiers/cap behavior, the S3 runbook matches collector semantics, -- since: 0.10.1 matches web/package.json, and English/Korean CHANGELOG sections were updated in parallel. No ADR amendment needed.

3. Suggestions

1. In _list_s3_buckets, treat an absent/None Buckets key as an empty page (fail closed only on wrong types), and either use the botocore list_buckets paginator or treat an echoed terminal-page token as end-of-list.
2. In modelClaim, accept IP-literal endpoints (or degrade invalid endpoints to null endpoint instead of discarding the claim) and add an unresolvedInference-style counter feeding partial status.
3. Change \d{12} → [0-9]{12} in _inference_claim_meta for exact TS/SQL parity.
4. Weight hosts edges at 1 (or exclude them from cap ranking) so execution-context nodes don't evict low-volume real dependencies.
5. Amend the existing CHANGELOG bullet (both languages) to name the inference-provenance migration and its standalone-application requirement.

4. Verdict

All findings across both panels are MINOR; the independent convergence on the endpoint-validation drop strengthens that finding but it remains a quality gap, not a correctness/security blocker. Read-only guarantees, ADR-005, claim provenance, and migration discipline are intact. The image manifest lists no images and no unavailable/omitted entries.

IMAGE_COVERAGE: NOT_REQUIRED

Triggered by commit 3923f375ccbce4cc74d434285510e78cdf447219 · workflow: .github/workflows/pr-review.yml